-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
📦 Add myst-execute package #866
Conversation
c510ed7
to
ed1ae93
Compare
|
5004c0d
to
716fc06
Compare
|
||
export type Options = { | ||
cache: ICache<(IExpressionResult | IOutput[])[]>; | ||
sessionFactory: () => Promise<SessionManager | undefined>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This maybe should return SessionManager
, but I don't have a feel for MyST's error handling yet, so erring on the side of "report this in the transform".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any error handling should be done through fileError
and fileWarning
and passing around a vfile.
Something like:
https://github.com/executablebooks/mystjs/blob/main/packages/myst-cli/src/transforms/code.ts#L159
This will likely also be adding a new RuleId
to keep track of these for later processing/ignoring/filtering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking another look through!
Looks like there is a merge conflict with the myst-common package and package.json after the release, we might need to rebase and just recommit the package-lock? Let me know if you need help! |
Co-authored-by: Rowan Cockett <[email protected]>
Co-authored-by: Rowan Cockett <[email protected]>
1e287b3
to
1b4cbba
Compare
Thanks for fixing this up @agoose77 -- do you think it is a state that it is ready to come in? |
Just want to tackle a few things RE cleanup first! |
@rowanc1 I removed the logging statements that were expected anyway --- this is good to go imo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be slow hitting merge on this - it looks great! Nice to have a clean, separate package we can start pulling into the CLI.
This just adds a basic skeleton package for
myst-execute
- a sandbox for the jupyter execution work introduced in #856@agoose77 - hopefully this helps get you started. Let me know if you run into problems or need a hand with anything...
@rowanc1 - I published to npm,
ebp-bot
should have an invite to enable CI publishing.